-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix pointer arguments in addFunction + wasm64 #24693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Without this fix, any usages of addFunction - e.g. dylink - would add broken functions to the webassembly table, which result in errors like "Cannot mix BigInt and other types, use explicit conversions" when invoked. To fix this, we need to do essentially the opposite of what we do in `dynCall` and convert all pointer arguments to numbers before invoking the actual JS function, and the pointer result to bigint before returning to wasm.
e08b2bd
to
3d8535f
Compare
} | ||
''') | ||
|
||
self.do_runf('main.c', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised we don't have any existing tests of addFunction being used with JS functions with 'p' in their signatures.
I'd like to take a moment to try and figure out why no existing test covered this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, as long as it doesn't block the fix too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it's possible that even if there were any tests, they 1) weren't doing pointer arithmetic and 2) weren't returning a pointer from JS to Wasm, only the other way around.
Without these conditions, e.g. if you have a simple tests that only logs the passed argument, the issues wouldn't trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just a little suprised we don't run into this issue with our exist dynamic linking tests. The main user or addFunction
for JS function with a signature passed are in dynamic linking, firstly in dlsym
and secondly (which I imagine occurs way more often) in reportUndefinedSymbols
which will fill in missing GOT entries using JS functions. I'm surprised we don't run into this there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait.. I think its because JS libraries functions are modified in place when they are constructed, right? So this change is not necessary for JS library functions at all, only for JS functions that come from elsewhere?
Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying trying to add non-js-library functions manually using
addFunction
with manual signature agument specified?
Yeah, or, well, not quite manual - I'm computing signature in Embind (#24611 was related to this). I'm trying to use addFunction
for another Embind-related PR and without this fix it would fail on wasm64 due to BigInt vs number incompatibility.
I can definitely fix it one level up, just for my own function, but as I said, I thought fixing it for other addFunction
users would be the right choice.
If you strongly feel it's not, I can leave this PR open and make that other PR with just a localized hotfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping - so what's the preferred course of action here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too keen on landing this, since its not necessary for JS libraries function (which are modified at code-generation time).
If you do want to continue to move forward with this we would need to make sure this code didn't create extra wrapping for JS library functions (perhaps by removing the 'p' elements from the .sig
which we generate at compile time). I'd also like to see this split out into a separate named function (e.g. wrapJSFunctionAndConvertPointerArgsAndParams
.. I'll leave it to you to come with this nicer name).
I'm tempted to say we shouldn't bother landing this since (I hope) mode usage of addFunction
is internal and not user-visible. Calling addFunction
from user code doesn't work in multi-threaded builds, for example (since the tables on different threads would then get out of sync).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough. Given that it's possible to use addFunction
from user code, and that it lives in its own libaddfunction.js
, I assumed it's part of public API, but it sounds like it's just one of many functions that isn't marked as __internal: true
for backward compat / legacy reasons rather than intended to be used by external developers?
If so, I'm happy to also close this and fix just my individual usage of addFunction
in the Embind PR where I needed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think its does have some users, but I'd like to discourage its usage if possible. I'd rather see dynamic dispatch mechanisms be built on the JS side instead of by injecting into (modifying the wasm table at runtime). (I think only the dynamic linker should be doing that really, but I've never really articulated that in any official docs anywhere).
Without this fix, any usages of
addFunction
with pointers in signatures would add broken functions to the webassembly table, which would fail with errors like "Cannot mix BigInt and other types, use explicit conversions" when invoked.To fix this, we need to do essentially the opposite of what we do in
dynCall
and convert all pointer arguments to numbers before invoking the actual JS function, and the pointer result to bigint before returning to wasm.